I was wondering why this wasn't internationalized and then I remembered that it's because this functionality isn't intended to be here long term. These aspects should probably be annotated with the TODO comments like you had in #3024 assuming that the decision is to continue having these as separate PRs. Alternatively, I think maybe this PR should have been in a Draft state until testing artifacts can be removed.

Why are these mocks initialized twice? Is one set redundant?

I was expecting this test to have something about clicking a cancel button in it. I think it's missing because it's using the browser confirmation instead of the modal. Just noting that this will probably ultimately need to be updated.

Please remove dangling comma.

This is correct JS syntax. The array has two items, I'm grabbing the second one. The first one is not saved as an unused variable - which the linter forbids - but instead omitted. Without the comma, the code will be incorrect.

Sorry, looking at it closer, maybe I'm used to seeing this syntax as const [_, setActiveActionMenuRowId] = useState<RowId | null>(null); to clarify, but it seems like either way is fine.

Apparently my suggestion fails the validation tests, so definitely nevermind on this.

I find this variable name to be a little confusing. What is an "AdditionalError"? I think isError corresponds to the error on create and the isAdditionalError is corresponding to the error on delete, but if that's the case then I think the variable names should be updated to be more explicit about what they represent. Alternatively, maybe they could be consolidated to something like hasMutationError that works for all of them?

I agree isAdditionalError isn't a very clear name here. If we're specifically using it for delete errors then isDeleteError could work. I do think that a future cleanup could be to replace the multiple error type flags with a single isError var that can be set to different error types, but I'd prefer that be a follow-up and not part of this PR.

I agree isAdditionalError isn't a very clear name here. If we're specifically using it for delete errors then isDeleteError could work. I do think that a future cleanup could be to replace the multiple error type flags with a single isError var that can be set to different error types, but I'd prefer that be a follow-up and not part of this PR.

Claude flagged this as a possible race condition, I asked Claude to elaborate and got this response:

enterViewMode(); // 1
await deleteTagMutation.mutateAsync({ ... }); // 2
setToast({ ... }); // 3

The intent (per the comment) is that view mode auto-reloads from the server when data changes, so after the delete succeeds and queries are invalidated, the table refreshes automatically without manually removing the row from the tree.

On success this works fine. The chain is: enter view mode → delete succeeds → onSuccess invalidates queries → refetch → useEffect rebuilds tree → deleted tag gone.

On failure it gets murkier. The catch block does:
enterViewMode(); // already happened
// ...mutation fails...
setDraftError(errorMessage);
setToast({ ... });

The SaveErrorAlert needs both isAdditionalError (from deleteTagMutation.isError) and draftError to be truthy — so the error should render. But the mode transition already happened before the failure, so the table is sitting in view mode looking completely normal while the request is in flight. There's no loading/pending state for the delete — the user gets no feedback that anything is happening, then an error alert appears seemingly out of nowhere.

The bigger concern is that nothing disables the UI during the delete. hasOpenDraft is false (we're in view mode, not draft mode), so all the action menu buttons remain enabled. The user could start an edit, create, or trigger another delete while the first delete is still in flight. If both complete, you get interleaved state updates.

So "race condition" was probably too strong — it's more of a missing in-progress state. The question for the PR author is: should there be some guard (even a simple deleteTagMutation.isPending check) that prevents other actions while a delete is in flight?

Could you elaborate on this a bit? If the frontend shows a tag without any subtags, but the backend has subtags for that tag, and then this runs, could that result in orphaned subtags on the backend?

I believe the backend guards against that case here: https://github.com/openedx/openedx-core/blob/6777915a956795ffd421baaeb9fc49ce50cad696/src/openedx_tagging/models/base.py#L691

I'll update the code comment to this for clarity:

    // Only request recursive deletion when the frontend has loaded descendants.
    // If this state is stale and the backend finds subtags while with_subtags is false,
    // the backend rejects the request instead of deleting the parent alone.